Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

libos/src/fs/sys/fs.c line 80 at r1 (raw file):

    while (1) {
        if (is_present(pos, callback_arg))
            word |= 1U << pos % 32;

This is a random thing triggered by a new UBSan version. I don't know when this check was added to UBSan, but the problem was simple: constant 1 is by default int i.e. signed integer. So 1 << 31 is technically undefined behavior (signed integer overflow). I didn't care to mention this in the commit message, as this is trivial.

libos/src/net/unix.c line 322 at r1 (raw file):

static int maybe_force_nonblocking_wrapper(bool force_nonblocking, struct libos_handle* handle,
                                           PAL_HANDLE pal_handle, bool is_pal_stream_read,

This UBSan complaint is actually interesting. Both PalStreamRead() and PalStreamWrite() have the correct function type. But UBSan still complained about a wrong type. I think it has to do with the fact that these are PAL-binary functions, and we do our own relocations in Gramine, so UBSan gets confused.

So my change is more like a workaround.

I thought about sending a bug report (a false positive) to UBSan developers, but that would require too much research how UBSan works and what it expects from linker/relocation code.

pal/src/pal_rtld.c line 407 at r1 (raw file):

    /* perform PLT relocs: supported binaries may have only R_X86_64_JUMP_SLOT relas */
    elf_rela_t* plt_relas_addr_end = (elf_rela_t*)((uintptr_t)plt_relas_addr + plt_relas_size);

Explanation: plt_relas_addr is a pointer, and it can be NULL here. Adding offset (in this case it is always 0) to a NULL pointer is undefined behavior. But we can cast the pointer to an integer, and then we're good. We use this trick here.

pal/src/host/linux-sgx/pal_main.c line 906 at r1 (raw file):

    init_handle_hdr(first_thread, PAL_TYPE_THREAD);
    first_thread->thread.tcs = (void*)((uintptr_t)g_enclave_base + GET_ENCLAVE_TCB(tcs_offset));

Explanation same as in above comment (g_enclave_base can legitimately be 0x0 aka NULL pointer)

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

-- commits line 11 at r1:
By "scaffolding", do you mean that this new feature is not fully supported after this PR?

Code quote:

This commit adds the scaffolding for the second (new) check

common/src/avl_tree.c line 523 at r1 (raw file):

 * UBSan complains about "Indirect call of a function through a function pointer of the wrong type"
 * because of this mismatch. However, allowing the first argument to be a pointer to any type was
 * done on purpose; see corresponding comment in avl_tree.h. So, silence this particular complaint.

I think UBSan is technically right, we can't do the call legally after such a cast. But I'm not sure whether it's worth fixing, I don't see any easy way to do that.

common/src/ubsan.c line 170 at r1 (raw file):

                                           value_handle function_pointer);
void __ubsan_handle_function_type_mismatch_abort(struct function_type_mismatch_data* data,
                                                 value_handle function_pointer);

What's the point of these forward declarations?

libos/src/libos_syscalls.c line 28 at r1 (raw file):

 * Variable syscall_func is of type six_args_syscall_t but points to item inside libos_syscall_table
 * array, which has a type libos_syscall_t, thus UBSan complains about "Indirect call of a function
 * through a function pointer of the wrong type". Silence this particular complaint.

ditto, we actually have UB, but it's not obvious how to fix it

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

-- commits line 11 at r1:

No, it is fully supported. I meant the "necessary support", but looks like I misused the English language. I'll replace with necessary support.

common/src/ubsan.c line 170 at r1 (raw file):

If remove them, you'll get warnings/errors like this:

[264/939] Compiling C object pal/src/host/skeleton/libpal.so.p/.._.._.._.._common_src_ubsan.c.o
../common/src/ubsan.c:145:6: warning: no previous prototype for ‘__ubsan_handle_type_mismatch_v1’ [-Wmissing-prototypes]
  145 | void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data* data, value_handle pointer) {
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../common/src/ubsan.c:158:6: warning: no previous prototype for ‘__ubsan_handle_type_mismatch_v1_abort’ [-Wmissing-prototypes]
  158 | void __ubsan_handle_type_mismatch_v1_abort(struct type_mismatch_data* data, value_handle pointer) {
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../common/src/ubsan.c:164:6: warning: no previous prototype for ‘__ubsan_handle_function_type_mismatch’ [-Wmissing-prototypes]
  164 | void __ubsan_handle_function_type_mismatch(struct function_type_mismatch_data* data,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../common/src/ubsan.c:177:6: warning: no previous prototype for ‘__ubsan_handle_function_type_mismatch_abort’ [-Wmissing-proto
types]

Compilers expect either static function or a prototype declared first. In here, we don't have a header file (we didn't bother with it, as it is really not required for anything). And we also can't declare the funcs as static since the UBSan instrumentation will try to look them up.

Hence this forward declaration.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

Reviewable status: all files reviewed, 5 unresolved discussions

common/src/ubsan.c line 170 at r1 (raw file):

Ah, I missed that there's no header for this file.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved